Repository navigation
Conversation
A namespace-import property with no matching export is rewritten to EImportIdentifier with import_item_status = Missing, which the printer replaces with undefined. Under --minify-syntax that emits 'void 0', and on the left of ** it was printed at Level::Exponentiation, below print_undefined's Level::Prefix wrap threshold, producing the SyntaxError 'void 0 ** 2'. Bump left_level to Call for EImportIdentifier when minify_syntax is on, mirroring the existing EBoolean arm. The non-Missing cases print an identifier or a member access, neither of which consults the incoming level, so the bump is a no-op there.
|
Reproduced with CI build 83526: the bundler test added here passes on every lane. The only red is |
WalkthroughThe JavaScript printer now applies call-level precedence to missing import identifiers on the left side of minified exponentiation. A bundler regression test verifies generated parentheses, warnings, and runtime output. ChangesExponentiation import identifier handling
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
LGTM — targeted 6-line printer fix following the existing EBoolean arm pattern.
What was reviewed:
- Confirmed
print_undefinedwraps only whenlevel >= Prefix, so bumpingleft_leveltoCallunderminify_syntaxcorrectly forces(void 0). - Checked the
EImportIdentifierprint path (src/js_printer/lib.rs:3932): non-missing branches emit an identifier or member access and never consultlevel, so the unconditional bump is a no-op there — matching the test'snot.toMatch(/\(\w+\) \*\* 2/)assertion. - Test covers LHS (wrapped), RHS (unwrapped, right-associative), and an existing export (unwrapped), plus runs the bundle and asserts the warning.
Extended reasoning...
Overview
This PR adds a single EImportIdentifier arm to the BinPow left-operand match in binary_check_and_prepare (src/js_printer/lib.rs). Under --minify-syntax, a namespace-import property with no matching export is printed as void 0 via print_undefined, which only parenthesizes when the incoming level is >= Prefix. Since ** is right-associative, the left operand's level was staying at Exponentiation (< Prefix), producing void 0 ** 2 — a SyntaxError. The fix bumps left_level to Call, mirroring the existing EBoolean arm two lines above (booleans print as !0/!1 under the same option and need the same treatment).
Security risks
None. This is a code-printer precedence adjustment for an edge case (missing import used as ** LHS). No parsing of untrusted input, no I/O, no allocation.
Level of scrutiny
Low. The change is 6 lines that copy the pattern of the adjacent EBoolean/EBranchBoolean arm exactly, gated on the same minify_syntax flag. I verified in the EImportIdentifier print handler at line 3932 that the non-missing branches (namespace alias → member access, or plain identifier) do not read the incoming level, so the unconditional bump cannot introduce spurious parentheses — the test's negative assertions (not.toContain("(2 ** void 0)"), not.toMatch(/\(\w+\) \*\* 2/)) confirm this. Without minify_syntax the missing import prints as the identifier undefined, which is already a valid ** LHS, so the minify_syntax gate is correct.
Other factors
The new itBundled test is well-constructed per the repo's bundler-test conventions: it runs the output bundle (proving no SyntaxError), asserts exact output shape for the wrapped LHS and unwrapped RHS/existing-export, and checks the "will always be undefined" warning. It sits alongside the existing NamespaceImportMissingES6 test in the right file. The PR description explains the mechanism precisely and references the sibling PR #36133 for the enum-inlining path. No outstanding reviewer comments.
|
@robobun is this exactly what esbuild does? |
|
No. esbuild 0.28.1 has the same bug: esbuild's This PR adds the missing arm in the same shape as the existing |
…n its assertions (#41028) ### Problem - `test/bundler/esbuild/importstar.test.ts` takes 11s on the debian 13 x64-asan lane (build 108747). The default `itBundled` backend is `api`, which the harness registers with `it.serial` (`expectBundled.ts:1962`), so its 75 cases run one at a time. 64 of them spawn a subprocess for `run`. - Three cases only check that the build succeeds. `ImportStarMangleNoBundleNoCapture` is a copy of the Capture case. ### Fix - Default the file to the CLI backend (the wrapper from `bundler_compile.test.ts`) and use `describe.concurrent`. Error and warning cases pass on it unchanged. - New checks: `ImportStarAndCommonJS` runs the bundle, no `__commonJS` wrapper. `ExportSelfIIFE` runs the bundle, no `__reExport` helper. `ImportExportOtherAsNamespaceCommonJS` requires the bundle and checks the JSON. `ImportStarNoCapture` and `ImportStarExportStarNoCapture`: no namespace object (`__export`). `ImportStarMangleNoBundleNoCapture` gets the upstream no-capture entry point. - Verified: `bun bd test test/bundler/esbuild/importstar.test.ts` (debug, ASAN): 35.3s and 35.4s before, 12.4s to 14.5s after (six runs). Released bun: 0.7s before, 0.25s after. 75 pass, 4 todo, every run. expect() calls: 64 before, 71 after. ### Background - The `api` backend calls `Bun.build()` in the test process with `process.chdir`, so the harness makes it serial. The `cli` backend spawns `bun build` in the case's own directory and can overlap. - `bun test` runs consecutive concurrent cases as one group of up to 20. A serial case ends the group, so the no-`run` cases also use `cli` (on `api` they measured 15.4s). - `__export` builds a namespace object. `__commonJS` wraps a CommonJS module. `__reExport` copies exports at runtime for `export *` of an external module. <details><summary>Notes</summary> What was not changed, and why: - `ImportStarExportStarAsNoCapture` and `ImportStarExportImportStarNoCapture` get no `__export` check. Bun still materializes the namespace for `ns.foo` through a re-export (esbuild binds `foo` directly). #41009 changes that and adds cases at the end of this file. This branch does not touch those lines. - `ImportDefaultNamespaceComboNoDefault1` to `4` stay separate builds. Bun reports only the first entry point's error, so one build with four entry points would check one error instead of four. - No case in the file sets `timeoutScale`. - No cases are grouped into shared builds. Each case has its own module graph with the same file names (`entry.js`, `foo.js`, `bar.js`). Other checks: - `bun bd test ... --todo`: the 4 todo cases still fail, as on main. - `BUN_BUNDLER_TEST_USE_ESBUILD=1`: main fails 32 cases because the API backend passes neither `format` nor `external` to `esbuild.build()`. This branch fails 2: `ImportStarCommonJSCapture` (esbuild adds `default` to the CommonJS namespace, noted in the case) and `ReExportStarEntryPointAndInnerFileExternal` (#37829). - Mutation check of the new output checks with the released bun: capturing `ns` in `ImportStarNoCapture` makes `__export` appear and the case fails. A CommonJS `foo.js` in `ImportStarAndCommonJS` makes `__commonJS` appear and the case fails. - Line 2 (the `itBundled` import) also changes in #36714 and #36134 (they add `expect` to line 1). The merge conflict is those two lines. - Debug ASAN runs after the change: 12.4s, 12.4s, 12.5s, 14.1s, 12.6s, 14.5s (runner's own `Ran 79 tests` line, host load average about 30). Released bun: 0.78s and 0.67s before, 0.25s and 0.24s after. </details> <!-- robobun:evidence:begin --> --- **[stamp-90s]** gate passed · iteration 0 · 1 files touched <details><summary>passes on PR (with fix)</summary> ```console Test-only change. Debug/ASAN (expected pass): $ bun bd test 'test/bundler/esbuild/importstar.test.ts' $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test test/bundler/esbuild/importstar.test.ts bun test v1.4.1 (a6c4cc2) test/bundler/esbuild/importstar.test.ts: (pass) bundler > importstar/ImportStarExportImportStarUnused [860.67ms] (pass) bundler > importstar/ImportStarCapture [933.82ms] (pass) bundler > importstar/ImportStarUnused [1362.73ms] (pass) bundler > importstar/ImportStarExportImportStarNoCapture [981.69ms] (pass) bundler > importstar/ImportStarNoCapture [1252.30ms] (pass) bundler > importstar/ImportStarExportImportStarCapture [737.57ms] (pass) bundler > importstar/ImportStarExportStarAsUnused [729.34ms] (pass) bundler > importstar/ImportStarExportStarAsNoCapture [754.71ms] (pass) bundler > importstar/ImportStarExportStarAsCapture [944.90ms] (pass) bundler > importstar/ImportStarExportStarUnused [767.40ms] (pass) bundler > importstar/ImportStarExportStarCapture [680.23ms] (pass) bundler > importstar/ImportStarCommonJSUnused [664.77ms] (pass) bundler > importstar/ImportStarExportStarNoCapture [791.44ms] (pass) bundler > importstar/ImportStarCommonJSNoCapture [701.31ms] (pass) bundler > importstar/ImportStarCommonJSCapture [782.90ms] (pass) bundler > importstar/ImportStarNoBundleUnused [414.74ms] (pass) bundler > importstar/ImportStarAndCommonJS [682.71ms] (pass) bundler > importstar/ImportStarNoBundleCapture [696.13ms] (pass) bundler > importstar/ImportStarNoBundleNoCapture [689.50ms] (pass) bundler > importstar/ImportStarMangleNoBundleUnused [726.10ms] (pass) bundler > importstar/ImportStarMangleNoBundleCapture [718.23ms] (pass) bundler > importstar/ImportStarMangleNoBundleNoCapture [640.67ms] (pass) bundler > importstar/ImportExportStarAmbiguousError [366.27ms] (pass) bundler > importstar/ImportStarExportStarOmitAmbiguous [723.51ms] (pass) bundler > importstar/ImportExportStarAmbiguous [604.14ms] (pass) bundler > importstar/ReExportStarNameCollisionNotAmbiguousImport [606.40ms] (pass) bundler > importstar/ReExportS ... (truncated) Exit: 0 ``` </details> <details><summary>diff hotspot</summary> ``` test/bundler/esbuild/importstar.test.ts | 44 ++++++++++++++++++++++++++++++--- 1 file changed, 40 insertions(+), 4 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests test/bundler/esbuild/importstar.test.ts 2 9 0 ``` </details> <!-- robobun:evidence:end -->
|
Folding this into #36124. That PR makes |
Repro
The linker emits a "will always be undefined" warning but still produces a bundle, and that bundle is a
SyntaxError(UnaryExpressionis not a valid left operand of**).Cause
A namespace-import property with no matching export is rewritten to
EImportIdentifierwithimport_item_status == Missing. The printer'sEImportIdentifierhandler callsprint_undefined(loc, level), which under--minify-syntaxemits unwrappedvoid 0whenlevel < Prefix. The BinPow left-operand match inbinary_check_and_preparehas noEImportIdentifierarm, soleft_levelstays atLevel::Exponentiation, and the output isvoid 0 ** 2.Fix
Add an
EImportIdentifierarm to the BinPow left-operand match, bumpingleft_leveltoCallwhenminify_syntaxis enabled. This mirrors the existingEBooleanarm (booleans print as!0/!1under the same option). When the import is not missing the printer emits an identifier or a member access, neither of which consults the incoming level, so the bump is a no-op there; and withoutminify_syntaxthe printer emits the identifierundefined, which is already a valid left operand.Sibling of #36133, which handles the enum-inlining path on the same operand.
Verification
New
importstar/NamespaceImportMissingES6ExponentiationLHSintest/bundler/esbuild/importstar.test.tsfails against the unfixed build (bundle containsvoid 0 ** 2and does not run) and passes with the fix. It also asserts no extra parentheses are added for the right operand or for an existing export.[stamp-90s] gate passed · iteration 0 · 2 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file
self-review · no surviving concerns
28 concerns were raised and did not survive verification.